-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: quince release support #47
Conversation
723bfcf
to
12a9919
Compare
hi @luisfelipec95, I hope you're doing well I've been testing this PR (I'm just testing the functionality and haven't seen the code yet), but something is going wrong with the execution; did you follow the Some examples:
|
Thanks, I already made a fix |
Thank you so much, @luisfelipec95, now the main functionality is working as expected. Now, reviewing the code & docs I have some comments:
|
Thank you very much!! I've already made the corrections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, @luisfelipec95.
- You update the chore requirements.
- Updated the Github actions.
- Update the requirements.
- Sorry for my late review, but I think we need to modify the constraints. If, in the constraint, we have something like djangorestframework<3.13.0 or opened-events==4.1.1, we are forced to use that version instead of the version by default from quince (restframework and events version), at least in our GitHub tests. I recommend looking at those constraints and maintaining only the necessary ones, trying to avoid the super strict constraints == (unless required).
- Update the readme and other doc.
- I think we need to update a little bit the format of the quince code block a (add lines between the content and the code-block because that info doesn't appear correctly in the readme), if it makes sense maybe group the release with similar config, for example Palm, you can say Palm and Quince, and that's it.
- The test cases work.
And again, sorry for not giving my feedback before.
Thank you very much for the feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @luisfelipec95. It looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments addressed & working as expected!
Thanks ✨
Description
This PR adds support for the Open edx Quince release.
Testing instructions
Aditional information
JIRA ISSUE DS-778